[Lint][AMDGPU] No store to const addrspace #109181

jofrn · 2024-09-18T19:40:41Z

Ensure store to const addrspace is not allowed by Linter.

llvmbot · 2024-09-18T19:41:14Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-amdgpu

Author: None (jofrn)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/109181.diff

2 Files Affected:

(modified) llvm/lib/IR/Verifier.cpp (+17)
(added) llvm/test/CodeGen/AMDGPU/const-store.ll (+9)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 06a67346fbf959..805c9ef7b72729 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -4224,6 +4224,19 @@ void Verifier::visitLoadInst(LoadInst &LI) {
   visitInstruction(LI);
 }
 
+static bool isConstantAddressSpace(unsigned AS) {
+  switch (AS) {
+    using namespace AMDGPUAS;
+    case CONSTANT_ADDRESS: case CONSTANT_ADDRESS_32BIT:
+    case CONSTANT_BUFFER_0: case CONSTANT_BUFFER_1: case CONSTANT_BUFFER_2: case CONSTANT_BUFFER_3:
+    case CONSTANT_BUFFER_4: case CONSTANT_BUFFER_5: case CONSTANT_BUFFER_6: case CONSTANT_BUFFER_7:
+    case CONSTANT_BUFFER_8: case CONSTANT_BUFFER_9: case CONSTANT_BUFFER_10: case CONSTANT_BUFFER_11:
+    case CONSTANT_BUFFER_12: case CONSTANT_BUFFER_13: case CONSTANT_BUFFER_14: case CONSTANT_BUFFER_15:
+    return true;
+    default: return false;
+  }
+}
+
 void Verifier::visitStoreInst(StoreInst &SI) {
   PointerType *PTy = dyn_cast<PointerType>(SI.getOperand(1)->getType());
   Check(PTy, "Store operand must be a pointer.", &SI);
@@ -4246,6 +4259,10 @@ void Verifier::visitStoreInst(StoreInst &SI) {
     Check(SI.getSyncScopeID() == SyncScope::System,
           "Non-atomic store cannot have SynchronizationScope specified", &SI);
   }
+  if (TT.isAMDGPU()) {
+    Check(!isConstantAddressSpace(SI.getPointerAddressSpace()),
+          "Store cannot be to constant addrspace", &SI);
+  }
   visitInstruction(SI);
 }
 
diff --git a/llvm/test/CodeGen/AMDGPU/const-store.ll b/llvm/test/CodeGen/AMDGPU/const-store.ll
new file mode 100644
index 00000000000000..9447ef9db59986
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/const-store.ll
@@ -0,0 +1,9 @@
+; RUN: not llc -mtriple=amdgcn < %s |& FileCheck %s
+
+define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) {
+; CHECK: Store cannot be to constant addrspace
+; CHECK-NEXT: store i32 %r, ptr addrspace(4) %out
+  %r = add i32 %a, %b
+  store i32 %r, ptr addrspace(4) %out
+  ret void
+}

github-actions · 2024-09-18T19:44:12Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Ensure store to const addrspace is not allowed by IR Verifier.

llvm/test/CodeGen/AMDGPU/const-store.ll

shiltian · 2024-09-19T04:27:40Z

llvm/lib/IR/Verifier.cpp

@@ -4224,6 +4224,33 @@ void Verifier::visitLoadInst(LoadInst &LI) {
  visitInstruction(LI);
 }

+static bool isConstantAddressSpace(unsigned AS) {


constant address space is not an AMD only thing. I'm thinking probably we can split this function into target dependent (for example, to query whether an AS is a constant AS) and independent part.

What would the independent part contain if we do not have a use case for it yet?

jayfoad · 2024-09-19T09:51:28Z

Was there any discussion about this? I'm not sure if it should be a verifier error or just undefined behaviour at run time. Also there are other instructions that write to memory, like atomicrmw.

arsenm

This must be UB, and not a verifier error. For example, we allow constant->flat casts. We do not want to have to consider the address spaces involved when folding something like store (addrspacecast ptr addrspace(4) %val to ptr)

arsenm · 2024-09-20T04:21:26Z

Plus long term I don't think we should have a constant address space at all.

jofrn · 2024-09-20T13:13:30Z

Was there any discussion about this? I'm not sure if it should be a verifier error or just undefined behaviour at run time. Also there are other instructions that write to memory, like atomicrmw.

It can be guarded with a flag perhaps. There was discussion to catch this in some way or another since it leads to more confusing error messages later on. Alternatively, we can have these checks go in a separate pass.

jofrn · 2024-09-20T13:19:57Z

This must be UB, and not a verifier error. For example, we allow constant->flat casts. We do not want to have to consider the address spaces involved when folding something like store (addrspacecast ptr addrspace(4) %val to ptr)

The addrspace in store (addrspacecast ptr addrspace(4) %val to ptr) won't be affected by getPointerAddressSpace since this function asks about the second operand to the store, store i32 %val, ptr addrspace(4) %ptr, in particular, ptr addrspace(4) %ptr. Your example gives a complex expression on the value source, the first operand.

nikic · 2024-09-20T13:28:23Z

These kinds of checks should go into the Lint pass instead of the Verifier pass.

arsenm · 2024-09-20T13:28:32Z

The addrspace in store (addrspacecast ptr addrspace(4) %val to ptr) won't be affected by getPointerAddressSpace since this function asks about the second operand to the store, store i32 %val, ptr addrspace(4) %ptr, in particular, ptr addrspace(4) %ptr. Your example gives a complex expression on the value source, the first operand.

You're reading the example too literally. It should be valid for a pass to strip away the cast, and store directly to the original address space of the pointer without considering any special information. It would be a target specific burden placed on any IR transformation, which is not OK.

This type of check belongs in Lint, which hardly ever gets used and I'm not sure is worth the effort

arsenm · 2024-09-20T18:40:20Z

llvm/test/CodeGen/AMDGPU/const-store.ll

+  %r = add i32 %a, %b
+  store i32 %r, ptr addrspace(4) %out
+  ret void
+}


test atomicrmw, cmpxchg, and a memset, memcpy? Also at least the addrspace(6) case

arsenm · 2024-09-20T18:40:36Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s
+
+define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) addrspace(4) {


no reason to set the address space of the function

arsenm · 2024-09-20T18:40:47Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s
+


negative test with a different target

arsenm · 2024-09-20T18:41:08Z

llvm/test/CodeGen/AMDGPU/const-store.ll

@@ -0,0 +1,9 @@
+; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - |& FileCheck %s


Should move this to test/Analysis/Lint

nikic

New approach looks reasonable to me.

nikic · 2024-09-21T07:33:25Z

llvm/lib/Analysis/Lint.cpp

+  if (TT.isAMDGPU()) {
+    switch(AS) {
+      using namespace AMDGPUAS;
+    case CONSTANT_ADDRESS:


Should this be a helper in AMDGPUAddrSpace.h? I'd expect this logic to already exist somewhere, but it looks like AMDGPUAA is only looking for CONSTANT_ADDRESS and nothing else:

llvm-project/llvm/lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp

Line 78 in 97aa8cc

if (LI->getPointerAddressSpace() == AMDGPUAS::CONSTANT_ADDRESS)

shiltian · 2024-09-21T15:43:41Z

llvm/lib/Analysis/Lint.cpp

@@ -401,6 +403,10 @@ void Lint::visitMemoryReference(Instruction &I, const MemoryLocation &Loc,
        "Unusual: Address one pointer dereference", &I);

  if (Flags & MemRef::Write) {
+    if (TT.isAMDGPU())
+      Check(!AMDGPU::isConstantAddressSpace(UnderlyingObject->getType()->getPointerAddressSpace()),


Is it possible or better to do the check via TTI? I'm not sure if it's a good idea to do the check based on triple.

This kind of verification doesn't really fit in with TTI

Just to be clear, the "check" I meant here was AMDGPU::isConstantAddressSpace.

llvm/test/Analysis/Lint/const-store.ll

arsenm · 2024-09-23T16:44:49Z

llvm/lib/Analysis/Lint.cpp

@@ -401,6 +403,10 @@ void Lint::visitMemoryReference(Instruction &I, const MemoryLocation &Loc,
        "Unusual: Address one pointer dereference", &I);

  if (Flags & MemRef::Write) {
+    if (TT.isAMDGPU())
+      Check(!AMDGPU::isConstantAddressSpace(UnderlyingObject->getType()->getPointerAddressSpace()),


This kind of verification doesn't really fit in with TTI

arsenm · 2024-09-23T16:45:17Z

llvm/test/Analysis/Lint/const-store.ll

+define amdgpu_kernel void @memcpy_to_const(ptr addrspace(6) %dst, ptr %src) {
+; CHECK0: Undefined behavior: Write to memory in const addrspace
+; CHECK0-NEXT: call void @llvm.memcpy.p6.p0.i32(ptr addrspace(6) %dst, ptr %src, i32 256, i1 false)
+  call void @llvm.memcpy.px.py.i32(ptr addrspace(6) %dst, ptr %src, i32 256, i1 false)


Use the actual address spaces, not x y

arsenm · 2024-09-24T04:44:57Z

llvm/include/llvm/Support/AMDGPUAddrSpace.h

+  switch (AS) {
+    using namespace AMDGPUAS;


Suggested change

switch (AS) {

using namespace AMDGPUAS;

using namespace AMDGPUAS;

switch (AS) {

llvm/test/Analysis/Lint/const-store.ll

arsenm

lgtm with nit

llvm/test/Analysis/Lint/const-store.ll

arsenm · 2024-09-24T15:09:16Z

llvm/test/Analysis/Lint/const-store.ll

+; CHECK0-NEXT: %void = atomicrmw add ptr addrspace(4) %dst, i32 %src acquire
+  %void = atomicrmw add ptr addrspace(4) %dst, i32 %src acquire
+  ret void
+}


Maybe test arbitrary call that may write (those probably should not fail )

Ensure store to const addrspace is not allowed by Linter.

jofrn requested a review from arsenm September 18, 2024 19:40

llvmbot added backend:AMDGPU llvm:ir labels Sep 18, 2024

jofrn requested a review from shiltian September 18, 2024 19:41

jofrn force-pushed the amdgpu-irverifier branch from 7a77ede to 5469cf1 Compare September 18, 2024 19:45

[Verifier][AMDGPU] No store to const addrspace

a099028

Ensure store to const addrspace is not allowed by IR Verifier.

jofrn force-pushed the amdgpu-irverifier branch from 5469cf1 to a099028 Compare September 18, 2024 19:57

shiltian reviewed Sep 19, 2024

View reviewed changes

arsenm requested changes Sep 20, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch from a82bcdc to b3fea0b Compare September 20, 2024 18:17

llvmbot added the llvm:analysis label Sep 20, 2024

arsenm reviewed Sep 20, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch 2 times, most recently from 0b69206 to b1c3d38 Compare September 20, 2024 19:52

[Lint] Switch check to Linter

4553484

jofrn force-pushed the amdgpu-irverifier branch from b1c3d38 to 4553484 Compare September 20, 2024 19:57

jofrn changed the title ~~[Verifier][AMDGPU] No store to const addrspace~~ [Lint][AMDGPU] No store to const addrspace Sep 20, 2024

jofrn force-pushed the amdgpu-irverifier branch from 14a876d to 2c640e5 Compare September 21, 2024 06:11

nikic reviewed Sep 21, 2024

View reviewed changes

Add negative test, move isConstantAddressSpace

393311c

jofrn force-pushed the amdgpu-irverifier branch from 2c640e5 to 393311c Compare September 21, 2024 15:35

llvmbot added the llvm:support label Sep 21, 2024

shiltian reviewed Sep 21, 2024

View reviewed changes

llvm/test/Analysis/Lint/const-store.ll Show resolved Hide resolved

jofrn force-pushed the amdgpu-irverifier branch from a3a013d to ed164f3 Compare September 23, 2024 16:15

arsenm reviewed Sep 23, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch 3 times, most recently from 8fa566a to 251171b Compare September 23, 2024 18:45

arsenm reviewed Sep 24, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch from 251171b to 56fa4e8 Compare September 24, 2024 13:45

arsenm approved these changes Sep 24, 2024

View reviewed changes

jofrn force-pushed the amdgpu-irverifier branch from 56fa4e8 to 5f9f282 Compare September 25, 2024 13:16

Added cmpxchg, atomicrmw, call to Lint

ae211fa

jofrn force-pushed the amdgpu-irverifier branch from 5f9f282 to ae211fa Compare September 25, 2024 15:43

jofrn merged commit 3e65c30 into llvm:main Sep 25, 2024
8 checks passed

augusto2112 pushed a commit to augusto2112/llvm-project that referenced this pull request Sep 26, 2024

[Lint][AMDGPU] No store to const addrspace (llvm#109181)

ef3d978

Ensure store to const addrspace is not allowed by Linter.

Sterling-Augustine pushed a commit to Sterling-Augustine/llvm-project that referenced this pull request Sep 27, 2024

[Lint][AMDGPU] No store to const addrspace (llvm#109181)

04c90f1

Ensure store to const addrspace is not allowed by Linter.

xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024

[Lint][AMDGPU] No store to const addrspace (llvm#109181)

12f6827

Ensure store to const addrspace is not allowed by Linter.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lint][AMDGPU] No store to const addrspace #109181

[Lint][AMDGPU] No store to const addrspace #109181

jofrn commented Sep 18, 2024 •

edited

Loading

llvmbot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

shiltian Sep 19, 2024

jofrn Sep 19, 2024

jayfoad commented Sep 19, 2024

arsenm left a comment

arsenm commented Sep 20, 2024

jofrn commented Sep 20, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading

nikic commented Sep 20, 2024

arsenm commented Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

arsenm Sep 20, 2024

nikic left a comment

nikic Sep 21, 2024

shiltian Sep 21, 2024

arsenm Sep 23, 2024

shiltian Sep 23, 2024

arsenm Sep 23, 2024

arsenm Sep 23, 2024

arsenm Sep 24, 2024

arsenm left a comment

arsenm Sep 24, 2024

		@@ -0,0 +1,9 @@
		; RUN: not opt -mtriple=amdgcn --passes=lint --lint-abort-on-error %s -o - \|& FileCheck %s

		define amdgpu_kernel void @store_const(ptr addrspace(4) %out, i32 %a, i32 %b) addrspace(4) {

[Lint][AMDGPU] No store to const addrspace #109181

[Lint][AMDGPU] No store to const addrspace #109181

Conversation

jofrn commented Sep 18, 2024 • edited Loading

llvmbot commented Sep 18, 2024 • edited Loading

github-actions bot commented Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayfoad commented Sep 19, 2024

arsenm left a comment

Choose a reason for hiding this comment

arsenm commented Sep 20, 2024

jofrn commented Sep 20, 2024 • edited Loading

jofrn commented Sep 20, 2024 • edited Loading

nikic commented Sep 20, 2024

arsenm commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jofrn commented Sep 18, 2024 •

edited

Loading

llvmbot commented Sep 18, 2024 •

edited

Loading

github-actions bot commented Sep 18, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading

jofrn commented Sep 20, 2024 •

edited

Loading